Skip to content

Conversation

@provokateurin
Copy link
Member

The download performance was really bad when implementing the largefile litmus tests in #513. I don't have exact numbers, but it seems like the speed is about 50-75% better with this. I assume that it either was doing memory copies (which get really slow once you have a big response body) or it just had a big overhead due to the iterator.

@Leptopoda
Copy link
Member

I could imagine this coming from the toList needing to iterate over every byte in the end.
We should probably use a generator function (sync* and yield).

How did you test this? I'd like to take a look on a few old devices that are severely memory limited.

@provokateurin
Copy link
Member Author

This is how I discovered it: https://github.com/provokateurin/nextcloud-neon/blob/d32084f192dd0da5493f8e2646749058baedcd3c/packages/nextcloud/test/litmus_test.dart#L231
I don't know if there is a good way to intentionally limit the memory of your dart process.

@Leptopoda
Copy link
Member

void main(final List<String> args) async {
  final largefileSize = pow(10, 8).toInt();

  final value = Uint8List(largefileSize);
  final stream = Stream.value(value.map((final e) => e).toList());

  final stopwatch = Stopwatch()..start();

  /// 2960ms
  final data = await stream.expand((final element) => element).toList();
  final result = Uint8List.fromList(data);

  /// 1250ms
  final result = Uint8List.fromList([
    for (final element in await stream.toList()) ...[
      ...element,
    ],
  ]);

  /// 859ms
  final result = Uint8List.fromList([
    for (final element in await stream.toList()) ...element,
  ]);

  /// 180 ms
  final buffer = BytesBuilder();
  for (final element in await stream.toList()) {
    buffer.add(element);
  }
  final result = buffer.toBytes();

  stopwatch.stop();
  print(stopwatch.elapsedMilliseconds);
}

not a scientific result by any means as I had my usual programs running in the background. Runs where consistent within 5% in subsequent runs.
I also want to highlight test 2 and 3 as we use this pattern in many places throughout the app. The last test was also the only one not causing an OOM kill with 10^9 as the size.

@provokateurin
Copy link
Member Author

Interesting! So it can be improved even more 👍

@Leptopoda Leptopoda force-pushed the fix/dynamite-response-body-speed branch from a8dc292 to 9a0eea5 Compare July 31, 2023 22:17
@Leptopoda
Copy link
Member

@provokateurin what do you think about this?

@provokateurin
Copy link
Member Author

LGTM

@provokateurin provokateurin merged commit ef3009f into main Aug 1, 2023
@provokateurin provokateurin deleted the fix/dynamite-response-body-speed branch August 1, 2023 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants